feat(low-code): pass top level params to PropertiesFromEndpoint requester #543
Conversation
📝 WalkthroughWalkthroughThe changes refine the parameter propagation logic in the manifest component transformer to support recursive propagation into nested components without an explicit "type" field. Two new helper methods detect and process nested components recursively. A new unit test verifies deep propagation of parameters and entity fields through a complex nested component structure. Changes
Possibly related PRs
Suggested reviewers
Would you like me to suggest adding anyone else to the reviewer list, or does this cover the relevant expertise, wdyt? Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
1-3822: Overall feedback on parameter inheritance approachThe code consistently implements parameter inheritance in all three places where nested components with parameters are created. This follows good software design principles by establishing a consistent pattern throughout the codebase.
To help future developers understand this pattern, would it be helpful to add a brief comment explaining the parameter inheritance? Maybe something like "# Merge parent model parameters into nested component parameters to allow parameter inheritance"? Wdyt?
🧰 Tools
🪛 GitHub Actions: Linters
[error] 1-1: Ruff formatting check failed. File would be reformatted. Run 'ruff format' to fix code style issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
[error] 1-1: Ruff formatting check failed. File would be reformatted. Run 'ruff format' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
2761-2763: Looks good - passing parameters from parent model to nested requesterThe code now merges parameters from the parent model into the nested requester component, which allows top-level parameters to be accessible in the PropertiesFromEndpoint requester. This is a nice improvement for parameter propagation.
2814-2818: Nice parameter inheritance implementation - parent params merge into property_list modelSimilar to the previous change, this properly passes parameters from the parent QueryProperties model to the nested property_list model. This ensures configuration values defined at the top level are available to the nested components, which is good for consistency.
One small thing to consider: What happens if there are parameter key collisions? Currently, parent parameters would override child parameters - is that the intended behavior? Wdyt?
3056-3062: I like this consistent pattern for parameter propagationFollowing the same pattern as the other changes, this passes parent model parameters to the query properties component when found in request parameters. This makes the parameter inheritance behavior consistent across all three locations.
The implementation properly handles the None case for parameters and maintains the same merging logic as the other instances.
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py (1)
202-207: Suggestion for _is_property_chunking_componentThe method correctly identifies components with a "QueryProperties" subcomponent. Would an early return pattern be cleaner here, wdyt?
@staticmethod def _is_property_chunking_component(propagated_component: Mapping[str, Any]) -> bool: - has_property_chunking = False for k, v in propagated_component.items(): if isinstance(v, dict) and v.get("type") == "QueryProperties": - has_property_chunking = True + return True - return has_property_chunking + return Falseunit_tests/sources/declarative/parsers/test_manifest_component_transformer.py (1)
465-571: Great test coverage for the property chunking feature!The test properly verifies deep parameter propagation through nested component structures including property chunking. It confirms that
$parametersandentityfields are correctly injected at every level.A suggestion that might make the test a bit more readable - would it make sense to create helper methods for building parts of these complex test structures? This could make future tests easier to construct as well, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py(2 hunks)unit_tests/sources/declarative/parsers/test_manifest_component_transformer.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py (2)
133-146: LGTM: Special handling for PropertyChunking componentsThe added logic properly addresses the case where a component without a "type" field might contain a "PropertyChunking" object that needs parameter propagation. This solves the issue of parameters not being passed to objects within request parameters.
209-226: LGTM: Property chunking processing methodThe method correctly processes components containing PropertyChunking objects by recursively propagating parameters. The implementation is clean and follows the pattern established elsewhere in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py (1)
216-223: Good addition for detecting nested components!This helper method clearly identifies when a component contains nested typed components. Would a more concise implementation using
any()be clearer, wdyt?- def _has_nested_components(propagated_component: Mapping[str, Any]) -> bool: - has_nested_components = False - for k, v in propagated_component.items(): - if isinstance(v, dict) and v.get("type"): - has_nested_components = True - return has_nested_components + def _has_nested_components(propagated_component: Mapping[str, Any]) -> bool: + return any(isinstance(v, dict) and v.get("type") for v in propagated_component.values())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py
[error] 149-149: mypy type checking error: invalid syntax
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py (4)
133-140: Clear improvement to handling JSON schema objects!The logic now specifically checks if we're dealing with a JSON schema object, handling both direct "object" type and nullable object types correctly. This makes the early return condition more precise.
211-214: Nice refinement to JSON schema object detection!The method now correctly handles both direct "object" type and the nullable variant ["null", "object"]. This makes the code more robust when dealing with different JSON schema formats.
224-241: Great implementation of nested component processing!This helper method effectively handles the recursive parameter propagation into nested components. The approach ensures that all nested components receive the appropriate parameters, maintaining consistency throughout the component tree.
141-167:❌ Incorrect review comment
Great enhancement for parameter propagation in nested components without types!
This addition nicely addresses the use case where a parent component doesn't have a type but contains nested components that should receive parameter propagation. The comment example helps clarify the intended use case.
There appears to be a mypy syntax error on line 149 in the comment example - the indentation in the YAML example might be causing issues with syntax checking. Would adjusting the indentation in the example help resolve the pipeline failure, wdyt?
🏁 Script executed:
#!/bin/bash # Check if the issue is with the comment indentation rg -n "type: QueryProperties" airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.pyLength of output: 145
Ignore the YAML indentation suggestion in comments—mypy doesn’t parse comments. It looks like the example is purely in a commented block, so adjusting its indentation won’t resolve any pipeline failures. Let’s investigate the actual error in the mypy logs to pinpoint the real issue.
Likely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Actions: Linters
[error] 149-149: mypy type checking error: invalid syntax
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py
Outdated
Show resolved
Hide resolved
darynaishchenko
deleted the
daryna/low-code/pass-parameters-to-properties-from-endpoint
branch
Summary by CodeRabbit
Refactor
Tests